-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store bootstrap parameters in sandbox metadata #9736
Conversation
Hi @abel-von. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
e1adf73
to
3aead05
Compare
3aead05
to
2b59409
Compare
2b59409
to
e4585b8
Compare
e4585b8
to
4fa9a37
Compare
4fa9a37
to
0f77030
Compare
/retest |
@abel-von: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Should we add an endpoint to query shim's address directly from the running shim instance? |
I think we may have to consider that every sandbox needs an endpoint for containerd to connect, no matter it is for Task API call, or other API(for example, Streaming API in #9965 ), It is because the shim sandbox controller implements by shim process so that the runtime related data like address becomes part of the sandbox metadata. For other customized sandbox controllers(may not implemented by shim), we may have no shim process, but we still serves some APIs so that containerd can manage Tasks or other stuff in sandbox, so maybe we can make this endpoint information part of sandbox's internal attributes. @mxpv |
@abel-von I don't object the need of this feature, there are many use cases we can benefit from this. I'm just not sure that going through metadata store as the right way to go. Why not to query shim endpoint directly from running shim instance? |
I think we have to get the endpoint first and then we can call the API of the running shim. Maybe we can add the address in the response of Status API of sandbox controller? |
0f77030
to
d2e942b
Compare
Removed the store of endpoint in sandbox db, please take a look again. @mxpv |
/ok-to-test |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
This is an impactful change, would want a few more LGTMs for awareness. |
Signed-off-by: Abel Feng <fshb1988@gmail.com>
Signed-off-by: Abel Feng <fshb1988@gmail.com>
Signed-off-by: Abel Feng <fshb1988@gmail.com>
Signed-off-by: Abel Feng <fshb1988@gmail.com>
Signed-off-by: Abel Feng <fshb1988@gmail.com>
a77a063
to
de38490
Compare
sandbox: Store bootstrap parameters in sandbox metadata and shim get them from sandbox metadata rather than other shim's bootstrap.json file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a desire to add convenience functions for endpoints? some structures and code paths are using the fields of the endpoint separately some are not.
Thoughts?
func (s *shim) Version() int { | ||
return s.version | ||
func (s *shim) Endpoint() (string, int) { | ||
return s.address, s.version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit.. seems like there is a desire to have endpoints be their own class.. but have not gone all the way with it ..
Start
sandbox.Extensions
of the Sandbox in db, then shim get the endpoint information from Sandbox Extensions and restore bootstrap parameters.This is the first PR of the sandboxed Task demonstrated in draft #9574